Skip to content

vfs: integrate with CJS and ESM module loaders#63653

Open
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:vfs-module-loader-integration
Open

vfs: integrate with CJS and ESM module loaders#63653
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:vfs-module-loader-integration

Conversation

@mcollina
Copy link
Copy Markdown
Member

Route loader fs and package.json operations through toggleable wrappers so the VFS can resolve and load modules from mounted paths.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 30, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 30, 2026

Codecov Report

❌ Patch coverage is 90.71207% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.32%. Comparing base (39b481b) to head (51b033a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/vfs/setup.js 85.54% 59 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63653      +/-   ##
==========================================
- Coverage   91.96%   90.32%   -1.64%     
==========================================
  Files         379      732     +353     
  Lines      166638   237271   +70633     
  Branches    25497    44727   +19230     
==========================================
+ Hits       153242   214325   +61083     
- Misses      13099    14656    +1557     
- Partials      297     8290    +7993     
Files with missing lines Coverage Δ
lib/internal/modules/cjs/loader.js 98.15% <100.00%> (+18.48%) ⬆️
lib/internal/modules/esm/get_format.js 94.83% <100.00%> (+21.03%) ⬆️
lib/internal/modules/esm/load.js 91.47% <100.00%> (+8.22%) ⬆️
lib/internal/modules/esm/resolve.js 99.04% <100.00%> (+11.14%) ⬆️
lib/internal/modules/helpers.js 98.75% <100.00%> (+7.70%) ⬆️
lib/internal/modules/package_json_reader.js 99.47% <100.00%> (+11.86%) ⬆️
lib/internal/vfs/setup.js 86.14% <85.54%> (-0.55%) ⬇️

... and 472 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina
Copy link
Copy Markdown
Member Author

@joyeecheung take a look, should be easier to review.

// internals may stop going through the JavaScript fs module entirely.
// Prefer module.registerHooks() or other more formal fs hooks released in the future.
source = fs.readFileSync(url);
source = loaderReadFile(url);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comment no longer applicable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of vfs is to provide a way to avoid monkey patching. I can update the comment to point to vfs.

const real = fs.realpathSync(path, {
[internalFS.realpathCacheKey]: realpathCache,
});
const real = toRealPath(path);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question.

mcollina added a commit to mcollina/node that referenced this pull request Jun 1, 2026
Restore the "DO NOT depend on the patchability" warnings in esm/load.js
and esm/resolve.js that were dropped along with the fs imports. The
warning still applies; it now also points at node:vfs as one of the
formal hook mechanisms callers should reach for instead.

Addresses review feedback from @jsumners-nr in
nodejs#63653
Route loader fs and package.json operations through toggleable
wrappers so the VFS can resolve and load modules from mounted paths.
When no VFS is mounted, the wrappers take a null-check fast path with
zero overhead.

Hooks:
- loaderStat / loaderReadFile / toRealPath / loaderLegacyMainResolve /
  loaderGetFormatOfExtensionlessFile in
  lib/internal/modules/helpers.js, consumed by cjs/loader.js,
  esm/resolve.js, esm/load.js and esm/get_format.js.
- loaderReadPackageJSON / loaderGetNearestParentPackageJSON /
  loaderGetPackageScopeConfig / loaderGetPackageType, consumed by
  package_json_reader.js.
- setLoaderFsOverrides / setLoaderPackageOverrides install / clear
  all hooks; clearRealpathCache exposes the helpers.js realpath
  cache so deregister can flush it.

lib/internal/vfs/setup.js installs the overrides on first
registerVFS and clears every JS-side loader cache (CJS _pathCache,
CJS stat cache, realpath cache, package.json cache) on every
deregister. The overrides themselves are uninstalled when the last
VFS is removed so the fast path is fully restored.
legacyMainResolve / extensionless-format behavior matches the C++
binding; package.json validation matches src/node_modules.cc
(silently omit non-string main, throw on non-string name/type, etc).

The "DO NOT depend on patchability" warnings in esm/load.js and
esm/resolve.js are preserved and now point at node:vfs and
module.registerHooks() as the formal hook mechanisms.

Tests cover require / import / module-hooks / package.json / cache
invalidation / cleanup-cycle scenarios under --experimental-vfs.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina mcollina force-pushed the vfs-module-loader-integration branch from 6321e08 to 51b033a Compare June 1, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants